misc: use-after-move vlog fixes & remove NOLINTNEXTLINE#30227
Merged
WillemKauf merged 6 commits intoredpanda-data:devfrom Apr 20, 2026
Merged
misc: use-after-move vlog fixes & remove NOLINTNEXTLINE#30227WillemKauf merged 6 commits intoredpanda-data:devfrom
misc: use-after-move vlog fixes & remove NOLINTNEXTLINE#30227WillemKauf merged 6 commits intoredpanda-data:devfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts several vlog() callsites to avoid bugprone-use-after-move warnings (by snapshotting values before moving the source object) and removes NOLINTNEXTLINE suppressions from the vlog macro definitions.
Changes:
- Snapshot/memoize values (e.g., offsets, sizes, group_id) before moving the underlying objects used to compute them.
- Update log formatting arguments to use the snapshotted values after moves.
- Remove
NOLINTNEXTLINEcomments fromsrc/v/base/vlog.hmacro definitions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/v/transform/rpc/client.cc |
Cache ids.size() before ids is moved into an async RPC lambda; use cached size in logs. |
src/v/storage/mvlog/segment_appender.cc |
Cache batch offsets before moving batch data into the entry body; use cached offsets in logs. |
src/v/kafka/server/group_manager.cc |
Cache group_id before moving kafka_r into offset_commit; use cached id in subsequent logs. |
src/v/cluster/rm_stm.cc |
Cache b.base_offset() before moving b into read_fence_batch; use cached offset in log. |
src/v/cluster/log_eviction_stm.cc |
Log using rp_start_offset/kafka_start_offset variables instead of moved-from val. |
src/v/base/vlog.h |
Remove NOLINTNEXTLINE suppressions from vlog macro definitions. |
Comments suppressed due to low confidence (1)
src/v/kafka/server/group_manager.cc:1081
first_erroris initialized toerror_code::nonebut is only assigned insideif (first_error != error_code::none), so it will never capture the first encountered partition error. This makes the laterif (first_error != error_code::none)check ineffective and can cause restore to report success despite per-partition errors. Flip the condition (or otherwise setfirst_erroron the first error) so errors are surfaced correctly.
group_id,
offsets_ntp,
kafka_p.error_code);
if (first_error != error_code::none) {
first_error = kafka_p.error_code;
}
WillemKauf
requested review from
a team,
BenPope,
andrwng and
dotnwat
and removed request for
a team
Collaborator
CI test resultstest results on build#83360
|
pgellert
approved these changes
Apr 20, 2026
andrwng
approved these changes
Apr 20, 2026
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Picked out of #30202. The changes to
vlog()there must have started picking up on some use-after-moves that aren't currently picked up by the linter because ofNOLINTNEXTLINE.It's a little curious why
NOLINTNEXTLINEis there anyways- lets remove it.Backports Required
Release Notes